Surface persistence exceptions to the caller more often#1348
Surface persistence exceptions to the caller more often#1348eric-maynard wants to merge 7 commits intoapache:mainfrom
Conversation
| * If this result is failed, this should throw the appropriate exception which corresponds to the | ||
| * result status. See {@link BaseResult#getException} for details. | ||
| */ | ||
| public void maybeThrowException() throws RuntimeException { |
There was a problem hiding this comment.
This helper method is nice to have because you can't throw an exception from a lambda, so you can't do e.g. result.getException().foreach(e -> throw e)
|
Unfortunately some Iceberg tests essentially assert that these methods don't throw exceptions, e.g. To pass these tests, we may want to gate this new behavior behind a flag |
eric-maynard
changed the title
eric-maynard
requested review from
MonkeyCanCode,
RussellSpitzer,
adutra,
ashvina,
collado-mike,
dennishuo,
dimas-b,
ebyhr,
flyrain,
jackye1995,
jbonofre,
snazy,
takidau and
vvcephei
as code owners
|
|
||
| String message = this.extraInformation; | ||
| if (this.extraInformation == null) { | ||
| // TODO this should ideally never be hit but it's hit often. |
There was a problem hiding this comment.
If we merge this, we should file an issue(s) tracking the many callsites where a non-successful result is built with a null message. This might be a good starter task.
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
github-project-automation
bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
1 participant
In many
...Catalogmethods likedropPolicyordropGenericTable, we simply returnfalseif there's aBaseResultfrom persistence that doesn't have aSUCCESSerror code. In some cases like inIcebergCatalogwe do a little better, mapping a small subset of failures into exceptions.This PR pushes this mapping down into
BaseResultso that methods which rely onBaseResultto report the success of a persistence call can pass accurate information about the error back to the caller (via the exception mapper in the case of a REST call).In order to support some Iceberg tests that expect methods like
dropTablenot to throw an exception, this functionality is gated behind a feature flagDETAILED_PERSISTENCE_EXCEPTIONS